-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use reserved identifiers memczero and benchmark_verify_t #835
Don't use reserved identifiers memczero and benchmark_verify_t #835
Conversation
As identified in bitcoin-core#829 and bitcoin-core#833. Fixes bitcoin-core#829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent.
FWIW http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2572.pdf I wonder if we can use clang-tidy or something like that to catch these UBs in the CI |
Just realized, Lines 269 to 270 in ac05f61
|
tACK e89278f |
Yeah, probably. We use IMO the entire libary would be more readable if we used typedefs like i32 and u128. But we'd get a huge diff now, and would interrupt all PRs, pretty much like clang-format. |
@real-or-random Replacing all int types everywhere is invasive, but I think can reasonably change |
I had thought the int128 typedefs only ran if they weren't already defined. I agree clashing here is not great. Personally I really don't like it when applications make local renames of all the generic standard types: Then I have to go digging through the code to make sure their custom type is defined to be the thing I think it is and that it's consistently the thing I think it is and isn't something different on some platform or another. Among other issues you end up with interface uglyness where your custom defined types end up in the interfaces and leak into applications. It was excusable back before there were standard fixed-size types, but isn't really today, and I think today it mostly still lingers on as copying a style that doesn't make sense anymore. This is distinct from e.g. making a type for a particular kind of data like a scalar or a field type, because at least that is defined to be fit for a particular purpose. In most cases using a for-application-type is preferable to using generic types (and, in fact, mandated pretty much universally by MISRA C). If a generic type needs to be renamed for some compatibility reason-- e.g. there isn't a universally available generic type with the required properties, then sure. |
ACK e89278f |
I added a commit that implements this. I also added Note: Currently there's no compiler that provides |
7d878a0
to
1f4dd03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to spell it out, the assumption is that a compiler that adds int128_t in the future also defines UINT128_MAX?
Looks good in general. Not sure how useful it is to define constants we don't use.
Yes. And this is implied by the standard as far as I understand it.
Yeah, one of the reasons why I decided for it is that it increases the probability to have a clash be apparent at compile-time (and fail compilation). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 1f4dd03
utACK 1f4dd03 |
…ify_t Summary: ``` As identified in #829 and #833. Fixes #829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent. Typedef (u)int128_t only when they're not provided by the compiler ``` Backport of [[bitcoin-core/secp256k1#835 | secp256k1#835]] Test Plan: ninja check-secp256k1 bench-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9373
…ify_t Summary: ``` As identified in #829 and #833. Fixes #829. Since we touch this anyway, this commit additionally makes the identifiers in the benchmark files a little bit more consistent. Typedef (u)int128_t only when they're not provided by the compiler ``` Backport of [[bitcoin-core/secp256k1#835 | secp256k1#835]] Test Plan: ninja check-secp256k1 bench-secp256k1 Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9373
As identified in #829 and #833. Fixes #829.
Since we touch this anyway, this commit additionally makes the
identifiers in the benchmark files a little bit more consistent.
This is necessary before we can merge #833. I preferred a separate PR because it makes it easier to see the results of Travis in #833.